Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for Rails 7.1 #686

Merged
merged 2 commits into from
Sep 30, 2023
Merged

Add support for Rails 7.1 #686

merged 2 commits into from
Sep 30, 2023

Conversation

yuki24
Copy link
Contributor

@yuki24 yuki24 commented Sep 24, 2023

This PR adds:

  1. A new gemfile and build matrix for Rails 7.1
  2. Fixes to address deprecation warnings
  3. Test updates to make it compatible with the latest Psych

I will leave comments to each change to provide more context. Addresses #685

yuki24
yuki24 commented Sep 24, 2023
View reviewed changes
lib/audited/audit.rb
@@ -49,7 +49,11 @@ class Audit < ::ActiveRecord::Base
cattr_accessor :audited_class_names
self.audited_class_names = Set.new

serialize :audited_changes, YAMLIfTextColumnType
if Rails.version >= "7.1"
serialize :audited_changes, coder: YAMLIfTextColumnType
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This addresses the deprecation warning below:

DEPRECATION WARNING: Passing the coder as positional argument is deprecated and will be removed in Rails 7.2.

Please pass the coder as a keyword argument:

  serialize :audited_changes, coder: Audited::YAMLIfTextColumnType
 (called from <class:Audit> at ~/lib/audited/audit.rb:55)

yuki24
yuki24 commented Sep 24, 2023
View reviewed changes
lib/audited.rb
@@ -22,7 +22,8 @@ def audit_class

# remove audit_model in next major version it was only shortly present in 5.1.0
alias_method :audit_model, :audit_class
deprecate audit_model: "use Audited.audit_class instead of Audited.audit_model. This method will be removed."
deprecate audit_model: "use Audited.audit_class instead of Audited.audit_model. This method will be removed.",
deprecator: ActiveSupport::Deprecation.new('6.0.0', 'Audited')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This addresses:

DEPRECATION WARNING: Module.deprecate without a deprecator is deprecated (called from singleton class at ~/lib/audited.rb:25)

yuki24
yuki24 commented Sep 24, 2023
View reviewed changes
spec/rails_app/config/application.rb
config.active_record.yaml_column_permitted_classes =
%w[String Symbol Integer NilClass Float Time Date FalseClass Hash Array DateTime TrueClass BigDecimal
ActiveSupport::TimeWithZone ActiveSupport::TimeZone ActiveSupport::HashWithIndifferentAccess]
end

if Rails.version >= "7.1"
config.active_support.cache_format_version = 7.1
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This addresses:

DEPRECATION WARNING: Support for `config.active_support.cache_format_version = 6.1` has been deprecated and will be removed in Rails 7.2.

Check the Rails upgrade guide at https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#new-activesupport-cache-serialization-format
for more information on how to upgrade.
 (called from <top (required)> at ~/spec/rails_app/config/environment.rb:5)

yuki24
yuki24 commented Sep 24, 2023
View reviewed changes
spec/support/active_record/models.rb
serialize :phone_numbers, Array

if Rails.version >= "7.1"
serialize :phone_numbers, type: Array
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This addresses:

DEPRECATION WARNING: Passing the class as positional argument is deprecated and will be removed in Rails 7.2.

Please pass the class as a keyword argument:

  serialize :phone_numbers, type: Array
 (called from <class:User> at ~/spec/support/active_record/models.rb:15)

yuki24
yuki24 commented Sep 24, 2023
View reviewed changes
spec/rails_app/config/application.rb
ActiveSupport::TimeWithZone,
ActiveSupport::TimeZone,
ActiveSupport::HashWithIndifferentAccess
]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently the elements of permitted classes need to be actual class objects rather than strings. Otherwise there would be a nuch of failing tests with the error message below:

  19) Audited::Auditor revisions should be able to get time for first revision
      Failure/Error: ActiveRecord::Coders::YAMLColumn.new(Object).dump(obj)

      Psych::DisallowedClass:
        Tried to dump unspecified class: ActiveSupport::TimeWithZone
      # ./lib/audited/audit.rb:30:in `dump'
      # ./lib/audited/auditor.rb:363:in `block in write_audit'
      # ./lib/audited/auditor.rb:362:in `write_audit'
      # ./lib/audited/auditor.rb:331:in `audit_create'
      # ./spec/audited/auditor_spec.rb:820:in `block (3 levels) in <top (required)>'

I have taken a look at Psych's code, and it does seem like that this has always been class objects rather than strings. Not sure where the original setting has derived, but either way, this should fix the test for Rails 7.1.

@danielmorrison
Copy link
Member

This is great. Thanks!

@danielmorrison danielmorrison merged commit 73aae10 into collectiveidea:main Sep 30, 2023
69 checks passed
@yuki24 yuki24 deleted the rails-7-1 branch September 30, 2023 13:33
@geetfun
Copy link

geetfun commented Oct 2, 2023

Just in time for 7.1 in a few days' time. Thank you @yuki24 and @danielmorrison for the release. 👏

@gjtorikian gjtorikian mentioned this pull request Oct 10, 2023
Closed
akostadinov
akostadinov reviewed Oct 10, 2023
View reviewed changes
lib/audited/audit.rb
@@ -49,7 +49,11 @@ class Audit < ::ActiveRecord::Base
cattr_accessor :audited_class_names
self.audited_class_names = Set.new

serialize :audited_changes, YAMLIfTextColumnType
if Rails.version >= "7.1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be better to use

Gem::Version.new(Rails.version) >= Gem::Version.new("7.1")

see

[8] pry(main)> Rails.version > "10.2.3"
=> true
[9] pry(main)> Gem::Version.new(Rails.version) >= Gem::Version.new("10.2.3")
=> false

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String version comparison is bad, agreed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing this out!
See proposal for its resolution: #689

@blaet blaet mentioned this pull request Nov 9, 2023
Merged
@phinze phinze mentioned this pull request Nov 10, 2023
Merged
phinze added a commit to chicago-tool-library/circulate that referenced this pull request Nov 13, 2023
@phinze
Rails 7.1 Upgrade (#1186)
8b34d23 
Fast on the heels of 7.0 here comes the latest and greatest Rails
version!

# What it does

Gets us on Rails 7.1.

# Why it is important

Latest is greatest. And specifically this gets us access to background
preprocessing of variants for finishing #1150

# Implementation notes

* This gets us off the fork of `audited` we were using. The [change we
switched for was
upstreamed](collectiveidea/audited#609) and we
need the [more recent work to make it compatible with Rails
7.1](collectiveidea/audited#686). Just a couple
of code changes were needed and tests seem to be passing okay.
* Updates us to [Devise
4.9.3](https://github.com/heartcombo/devise/blob/main/CHANGELOG.md#493---2023-10-11)
which added Rails 7.1 support.
- Note Devise 4.9.0 [added Hotwire + Turbo
support](https://github.com/heartcombo/devise/blob/main/CHANGELOG.md#490---2023-02-17),
something we might want to take a look at in either #1184 or a follow on
task.
* Includes a bump to Postgres 15 for the Docker and CI setups to match
production.
* The new Rails defaults in 7.1 raise in controllers when you mention an
action that is not defined. This caught a couple of `before_action`
typos.
* Running `rake app:update` yielded diffs on many of the config files. I
took all the changes to `bin` scripts whole hog and merged the rest. A
second set of eyes across those diffs would be helpful though! 👀
* 💥 The upgrade caused an issue with threaded background work in tests
whose fix is at the bottom of `config/environments/test.rb` with a note
explaining.
* 💡 I had neglected to update the Yarn dependencies for Rails in the 7
upgrade, so catching those up here.
* 🚧 Asset compilation now runs on every execution of `bin/rails test`
which made the SCSS deprecation warnings get pretty annoying. Pulling in
a workaround from picturepan2/spectre#694
* Zeitwork (the new code autoloader) did not like
`lib/rails_solargraph.rb`, so following a [comment on the referenced
gist](https://gist.github.com/castwide/28b349566a223dfb439a337aea29713e?permalink_comment_id=4346127#gistcomment-4346127)
I switch us to the solargraph-rails gem.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akostadinov akostadinov akostadinov left review comments

@blaet blaet blaet left review comments

@pboling pboling pboling left review comments

@danielmorrison danielmorrison danielmorrison approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@yuki24 @danielmorrison @geetfun @pboling @blaet @akostadinov